Add support for Optional<T> in Dart generator (both dart and dart-dio) to distinguish absent, null, and present states#22257
Conversation
…io) to distinguish absent, null, and present states
…ns thing for testing (setting both options to "true" for both types)
|
thanks for the pr please follow step 3 to update the samples |
Awesome. I will look into it later today 👍 |
cdfa8fc to
57a8568
Compare
|
👌 let us know if you need any help. |
… - match previously generated setup
|
cc @jaumard (2018/09) @josh-burton (2019/12) @amondnet (2019/12) @sbu-WBT (2020/12) @kuhnroyal (2020/12) @agilob (2020/12) @ahmednfwela (2021/08) for review |
amondnet
left a comment
There was a problem hiding this comment.
- Add behavioral tests for:
- useOptional flag wrapping non-required properties
- patchOnly mode PATCH schema detection
- patchOnly=true auto-enabling useOptional
- Parameter unwrapping behavior
- Fix the logic gap where useOptional=true without patchOnly=true appears to do nothing
- Rename getString() to extractModelNameFromBodyParam() or similar
* useOptional flag wrapping non-required properties * patchOnly mode PATCH schema detection * patchOnly=true auto-enabling useOptional * Parameter unwrapping behavior
There was a problem hiding this comment.
5 issues found across 19 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="modules/openapi-generator/src/main/resources/dart2/serialization/native/native_class.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/dart2/serialization/native/native_class.mustache:278">
P1: Inconsistent Optional handling for integer types: uses `json[...] == null` check instead of `json.containsKey()`. This conflates 'key absent' with 'key present but null value', defeating the purpose of Optional<T> to distinguish these states. Other Optional cases in this template correctly use `containsKey()`.</violation>
<violation number="2" location="modules/openapi-generator/src/main/resources/dart2/serialization/native/native_class.mustache:282">
P1: Inconsistent Optional handling for number types: uses `json[...] == null` check instead of `json.containsKey()`. This conflates 'key absent' with 'key present but null value', defeating the purpose of Optional<T> to distinguish these states.</violation>
</file>
<file name="modules/openapi-generator/src/main/resources/dart/libraries/dio/optional.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/dart/libraries/dio/optional.mustache:130">
P1: The `fromJson` method cannot distinguish between an absent field and a field explicitly set to `null`. When JSON contains `{"field": null}`, this returns `Optional.absent()` instead of `Optional.present(null)`, defeating the core purpose of the Optional class.</violation>
</file>
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractDartCodegen.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractDartCodegen.java:775">
P2: Adding schemas to `patchRequestSchemas` here has no effect because `postProcessModels` has already executed before `postProcessOperationsWithModels`. The `preprocessOpenAPI` method already handles PATCH schema identification at the right time. This code block is effectively dead code that misleads about its functionality.</violation>
<violation number="2" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractDartCodegen.java:813">
P2: Incorrect model name extraction for `Map<K, V>` types. This will extract `String, MyModel` instead of just `MyModel` for `Map<String, MyModel>`, causing the wrong schema name to be added to `patchRequestSchemas`. Consider parsing only the value type for Map (e.g., split by comma and take the last part).</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
|
The stuff that the cubic-dev-ai bot thing is talking about seems valid. Should I fix those? |
… executed before postProcessOperationsWithModels. And then we don't even need the extractModelNameFromBodyParam method...
Had issues in dio
|
Ok I went through the bot stuff and solved them. The containsKey: Inconsistency fixed. I see the files has different patterns and I've just mixed them. My blames should be good now though. |
|
Great PR @eirikb - I'd love to see this one get merged! Are you still actively working on it? |
As far as I know we are waiting for the reviewers to accept, unless I'm mistaken. |
|
thanks for the PR will get this reviewed and merged soon. (sorry for the delay as there are too many PRs...) |
|
Any update on this? Would be great to see it merged 😃 |
|
sorry please resolve the merge conflicts and 'll get it merge this weekend. |
…ure/eirikb/dart-optional-patch # Conflicts: # modules/openapi-generator/src/main/resources/dart2/serialization/native/native_class.mustache
|
Did a fetch of upstream master in IntelliJ and did a merge. Looks like it worked - but I got a bunch of newline changes. No idea why. I'll try to clean those up. |
|
Done |
|
PR merged. please test the latest and let us know how that goes: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-to-test-with-the-latest-master-of-openapi-generator |
…io) to distinguish absent, null, and present states (OpenAPITools#22257) * Add support for `Optional<T>` in Dart generator (both dart and dart-dio) to distinguish absent, null, and present states * Add useOptional and patchOnly options to the Dart client configurations thing for testing (setting both options to "true" for both types) * Add documentation for useOptional and patchOnly options * Tune the dart mustache (pluss class mustache) to get rid of the extra whitespace * More tuning of the dart mustache files to adjust amount of whitespace - match previously generated setup * Tune dart mustache templates to fix whitespace stuff by tips from wing328 * Fix the logic gap where useOptional=true without patchOnly=true appears to do nothing * Rename getString() to extractModelNameFromBodyParam() * Add behavioral tests * useOptional flag wrapping non-required properties * patchOnly mode PATCH schema detection * patchOnly=true auto-enabling useOptional * Parameter unwrapping behavior * Fix inconsistency (my own) in native_class.mustache * Remove "dead code" (because of timing). postProcessModels has already executed before postProcessOperationsWithModels. And then we don't even need the extractModelNameFromBodyParam method... * Fix Optional<T> to properly distinguish between absend and null Had issues in dio * Regenerate Dart samples * Fix extra blank lines in dart-dio json_serializable template output

fixes #21826
I saw in the guidelines that you guys don't want too many flags, well, here are two more:
useOptional=true: Enable Optional support all over, andpatchOnly=true: Automatically apply Optional only to only PATCH request schemasThe second one there might seem silly, but it makes a lot of sense to only support absent fields for PATCH.
I had a look at the Java
Nullablething and there it seems it is applied all over, at least when I generated.This might be fine, but not always.
Without the flags the generated stuff should be as before.
This PR require Dart 3 Optional, as I use this to make fields optional.
PR checklist
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)@josh-burton
@yissachar
Some details
Run like this:
Will produce output like this:
Without the
patchOnly=trueflag all fields will beOptional.Thanks :)